feat: use external package for ordered maps #1797
Conversation
pd93
requested review from
vmaerten and
andreynering
and removed request for
vmaerten
There was a problem hiding this comment.
Great description 👏 and great PR. It fixes the issue and tidy up our code.
I totally agree with you on those 3 points. Also the author seems to be reactive (as he already answered one of your issue)
Globally happy with this changes.
Looking forward to be able to use the iterator 😄
|
Please don't do it by just changing all This ordered maps doesn't have mutex and it's not goroutine safe, so we still need to add mutex to fix fatal error: concurrent map writes in the future, and that's why we change omap in the first please. This means we will still need to add mutex to ordered map struct in the future. and it's much easier to add it with If you don't want to add mutex first, we still should change internal implment of |
|
@trim21 I'm working on a separate branch to add the mutexes. I didn't want to make this PR even larger than it already is though. I want to get both merged before we release again. |
swap internal implement of omap will only make it smaller, not bigger. |
I mean I don't want to make this PR bigger by adding the mutexes to the implementations now. It will make the refactor and the fix harder to review. I intend to release both at the same time. I do not want to have an internal omap package for the reasons stated in the PR description. |
This PR completely removes and replaces our native
omappackage with a 3rd-party one. There are a few reasons for this change:This PR also makes way for the use of iterators (which we cannot use until we bump our minimum version to 1.23). A working PoC of this is available in #1798.
This also has a direct knock-on effect to #1605 - though falls short of fixing it. @trim21 has #1701 open to address this. I'm afraid that this will need rethinking as the fix was in our custom
omapimplementation previously. Now that we have a bit more separation, we will need to bring the fixes into our implementations ofTasks,IncludesandVars. This falls inline with the thoughts of the creator of theorderedmappackage we are moving to. That is, concurrency should be fixed by the implementation, not the ordered map itself. This is similar to how a Gomapdoes not have built-in concurrency protection.As for the code, I chose elliotchance/orderedmap for a couple of reason over other packages:
O(1)overSet,Get,DeleteandLenoperations.I have opened a couple of issues on the project that would ease our integration. However, neither of there are dealbreakers - Just nice-to-have:
One additional note. We now use custom unmarshalling on all implementations of ordered maps (previously this was only on
Includes). This is because theorderedmappackage does not natively support is. It may do in the future, but this doesn't matter to us as the custom unmarshal functions actually tidy up some of our code. Generally speaking, I have tried to tidy up and make all the implementations of ordered maps in our code a bit more consistent.